🤖 Kimi Code Review

Overall Assessment: The PR implements block retrieval RPC endpoints with proper error handling and comprehensive tests. The implementation is generally sound, but there is a critical missing dependency and a few minor issues.

Critical Issues

1. Missing hex dependency in types crate

• File: crates/common/types/Cargo.toml (not shown in diff, but required)
• Issue: crates/common/types/src/attestation.rs uses hex::encode (line 78), but hex is only added to crates/net/rpc/Cargo.toml. This will cause a compilation error.
• Fix: Add hex.workspace = true to crates/common/types/Cargo.toml.

Code Correctness & Safety

2. Potential panic on database corruption in store.rs

• File: crates/storage/src/store.rs, lines 928-937
• Issue: Uses .expect("valid header") and .expect("valid body") when decoding SSZ from storage. While consistent with existing code, corrupted database entries

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

🤖 Codex Code Review

1. Current-head slot lookup is incorrect. In blocks.rs:72, resolve_slot reads only head_state.historical_block_hashes[slot]. But process_block_header only materializes parent roots there and explicitly stops at block.slot - 1 in state_transition/lib.rs:174 and state_transition/lib.rs:188. That means GET /lean/v0/blocks/<head_slot> returns 404 for the actual canonical head, and slot 0 also fails on a fresh anchor-state store. This should special-case slot == head_state.latest_block_header.slot and resolve to store.head().

2. Slot resolution is much more expensive than it needs to be and creates an easy RPC DoS surface. blocks.rs:73 calls store.head_state(), which clones/deserializes the full head state from storage in store.rs:1217 just to read one historical root. On a real node that means pulling validators, justification bitfields, and historical roots for every unauthenticated GET /blocks/<slot> request. A lightweight accessor for just the head header/root history, or a dedicated slot-to-root index, would avoid turning this endpoint into O(state size) work.

3. The hex-root parser allocates before validating length. In blocks.rs:62, hex::decode(hex_body) runs on attacker-controlled path input, and only afterward checks that the decoded length is 32 bytes. A large 0x... path will force a proportional allocation before rejection. Check hex_body.len() == 64 first, then decode.

No consensus-transition, fork-choice, attestation, XMSS, or SSZ-state-machine logic changed here; the concerns are confined to the new RPC path and its storage access patterns.

I could not run cargo test -p ethlambda-rpc in this sandbox because Cargo/Rustup needed to write under read-only home directories and fetch network dependencies.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

🤖 Claude Code Review

Now I have enough context for a thorough review.

PR #301 Review: Add JSON-RPC endpoints for blocks

Overview

Adds GET /lean/v0/blocks/{block_id} and GET /lean/v0/blocks/{block_id}/header to the API server. Introduces Serialize on core block/attestation types, a new Store::get_block method that handles the genesis case, and 7 unit tests covering happy paths and error paths.

The implementation is clean and consistent with project conventions. A few issues worth addressing before merging.

Correctness Issues

The head block is unreachable by slot number (blocks.rs:68–78)

historical_block_hashes is populated by the STF when processing a block at slot N — it pushes the parent's root. That means historical_block_hashes[N-1] = parent_root, and the head block itself (at the current head slot S) is never pushed as an entry. Consequently, GET /lean/v0/blocks/{head_slot} will always return 404.

This is a silent correctness gap: a caller who queries the latest slot by number gets a confusing 404 even though the block exists. The simplest fix is to special-case the head slot:

fn resolve_slot(store: &Store, slot: u64) -> Result<H256, BlockIdError> {
    let head_state = store.head_state();

    // historical_block_hashes contains entries for slots 0..head_slot-1.
    // The head block itself must be retrieved from latest_block_header.
    if slot == head_state.latest_block_header.slot {
        return Ok(head_state.latest_block_header.hash_tree_root());
    }

    let root = head_state
        .historical_block_hashes
        .get(slot as usize)
        .ok_or(BlockIdError::NotFound)?;
    if root.is_zero() {
        return Err(BlockIdError::NotFound);
    }
    Ok(*root)
}

At minimum, document the limitation clearly in the function doc comment. A test for GET /lean/v0/blocks/{head_slot} would surface this immediately.

Minor Issues

slot as usize truncation (blocks.rs:75)

On 32-bit platforms slot: u64 truncates silently when cast to usize. The same pattern already appears in state_transition/src/lib.rs:261, so this is consistent — but consider adding a debug_assert!(slot <= usize::MAX as u64) or using usize::try_from(slot).map_err(|_| BlockIdError::NotFound)? for robustness.

parse_root style (blocks.rs:56–63)

The manual copy_from_slice can be replaced with try_into:

fn parse_root(hex_body: &str) -> Result<H256, BlockIdError> {
    let arr: [u8; 32] = hex::decode(hex_body)
        .map_err(|_| BlockIdError::Invalid)?
        .try_into()
        .map_err(|_| BlockIdError::Invalid)?;
    Ok(H256(arr))
}

serialize_attestations verbosity (block.rs:200–210)

The custom serializer can be replaced with one line:

fn serialize_attestations<S>(attestations: &AggregatedAttestations, serializer: S) -> Result<S::Ok, S::Error>
where S: Serializer {
    serializer.collect_seq(attestations.iter())
}

head_state() clones the full state per request (blocks.rs:71)

Slot-based lookups clone the entire head state (validators, justified_slots, etc.) just to read one element of historical_block_hashes. For the current scale this is fine, but it's worth noting if the endpoint ever becomes hot. No change needed now.

Test Coverage Gap

The /header endpoint is only tested by root (get_block_header_by_root_returns_json). There is no test for GET /lean/v0/blocks/{slot}/header. The slot-resolution path for the header variant is exercised by the slot body test, but symmetry with the existing test matrix suggests adding one.

What's Done Well

• get_block correctly handles the genesis case (no BlockBodies entry for blocks with empty bodies) using the EMPTY_BODY_ROOT check — consistent with get_signed_block.
• serialize_aggregation_bits correctly encodes the SSZ representation (including the sentinel 1 bit) as 0x-prefixed hex, matching the beacon API convention.
• insert_block_raw in test utilities correctly mirrors the production storage contract.
• BlockIdError::into_response correctly composes the status override on top of json_response without double-allocating the body.
• Error path coverage (400, 404 variants) is thorough.

Automated review by Claude (Anthropic) · sonnet · custom prompt

Greptile Summary

This PR adds two debug HTTP endpoints (GET /lean/v0/blocks/{block_id} and GET /lean/v0/blocks/{block_id}/header) to the RPC server, along with serde::Serialize implementations for Block, BlockBody, AggregatedAttestation, and AttestationData, and a new Store::get_block method that handles the genesis block case correctly. The implementation is well-tested with coverage of happy paths (by root, by slot) and error paths (400 invalid id, 404 missing/empty slots, 404 unknown root).

Confidence Score: 5/5

Safe to merge — no correctness issues; all two findings are P2 style suggestions.

The block endpoint logic is correct, error handling covers all edge cases (400/404), genesis block is handled properly via EMPTY_BODY_ROOT, and tests cover 7 distinct paths. Only P2 suggestions remain: full-state deserialization cost in slot lookup and a missing slot-based header test.

No files require special attention.

Sequence Diagram

sequenceDiagram
    participant Client
    participant API as RPC API Server
    participant Blocks as blocks.rs
    participant Store as Store
    participant DB as RocksDB Backend

    Client->>API: GET /lean/v0/blocks/{block_id}
    API->>Blocks: get_block(block_id)

    alt block_id starts with "0x"
        Blocks->>Blocks: parse_root(hex_body)
        Blocks-->>Blocks: H256 root
    else block_id is all digits
        Blocks->>Store: head_state()
        Store->>DB: get(Metadata, "head")
        DB-->>Store: head root
        Store->>DB: get(States, head_root)
        DB-->>Store: State bytes (incl. historical_block_hashes)
        Store-->>Blocks: State
        Blocks->>Blocks: historical_block_hashes[slot]
        alt slot out of bounds or zero root
            Blocks-->>Client: 404 Not Found
        end
        Blocks-->>Blocks: H256 root
    else invalid
        Blocks-->>Client: 400 Bad Request
    end

    Blocks->>Store: get_block(root)
    Store->>DB: get(BlockHeaders, root)
    DB-->>Store: header bytes
    alt body_root == EMPTY_BODY_ROOT
        Store-->>Store: BlockBody::default()
    else
        Store->>DB: get(BlockBodies, root)
        DB-->>Store: body bytes
    end
    Store-->>Blocks: Option<Block>

    alt block found
        Blocks-->>Client: 200 OK (JSON)
    else
        Blocks-->>Client: 404 Not Found
    end

This is a comment left during a code review.
Path: crates/net/rpc/src/blocks.rs
Line: 72-82

Comment:
**Full state deserialization for a single slot lookup**

`store.head_state()` deserializes the entire beacon `State` — including `historical_block_hashes`, which grows linearly with the chain's age (32 bytes × N slots). On a long-running chain this can be hundreds of MB of SSZ parsing just to index one entry. Since this is a debug endpoint the cost is tolerable for now, but a dedicated `Store::get_historical_block_hash(slot: u64) -> Option<H256>` accessor that reads only the `States` entry and pulls the specific index would eliminate the overhead without much extra complexity.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: crates/net/rpc/src/lib.rs
Line: 356-369

Comment:
**Missing test for slot-based header lookup**

`get_block_by_slot_returns_json` covers `GET /lean/v0/blocks/{slot}` but there is no corresponding test for `GET /lean/v0/blocks/{slot}/header`. The `store_with_historical_block` fixture is already set up — a quick additional case would confirm the slot→root→header path works end-to-end.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "Add JSON-RPC endpoints for blocks (close..." | Re-trigger Greptile}